-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: discovery fail when at least one api is unavailable #2732
fix: discovery fail when at least one api is unavailable #2732
Conversation
Welcome @rossignolloic! |
b9f9c8f
to
c5dcaa0
Compare
V1APIResourceList resourceList = resourceDiscovery(path); | ||
return groupResourcesByName(group, versions, preferredVersion, resourceList); | ||
} catch (ApiException e) { | ||
LOGGER.warn("Api {} returns a {} code, all api resources managed by this api cannot be discovered", path, e.getCode(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than doing this here, which I think is going to confuse a developer who uses this to actually call the discovery API, can you add the catch in the Kubectl
code that is executing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the suggestion is to ignore the ApiException in this code:
java/extended/src/main/java/io/kubernetes/client/extended/kubectl/Kubectl.java
Lines 235 to 238 in c8923e6
try { | |
ModelMapper.refresh(new Discovery(apiClient)); | |
} catch (ApiException e) { | |
throw new KubectlException(e); |
If I ignore the exception at this location, when an api raises an error, this code will never be executed:
java/util/src/main/java/io/kubernetes/client/util/ModelMapper.java
Lines 275 to 282 in c8923e6
addModelMap( | |
apiResource.getGroup(), | |
version, | |
apiResource.getKind(), | |
apiResource.getResourcePlural(), | |
apiResource.getNamespaced(), | |
objClass, | |
objListClass); |
Then, this method will always return null:
public static GroupVersionResource getGroupVersionResourceByClass(Class<?> clazz) { |
And we will get the folowing exception when executing the client request:
throw new KubectlException("Unexpected unknown resource type: " + apiTypeClass); |
If I'm not mistaken, this suggestion does not fix the initial issue.
What do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initModelMap()
is always called statically:
static { |
So that still should work correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, concretely, the suggestion is not to ignore all ApiExceptions, just to ignore this specific exception using the HTTP code/message to detect it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initModelMap()
is always called statically:
static { So that still should work correctly.
Unfortunatly, the static initModelMap is not enougth to make extended client work without the discovery. This method does'nt init vars classesByGVR, and as demonstrated earlier, when this vars is not set, the extended client will lead to throwing another exception.
Also, concretely, the suggestion is not to ignore all ApiExceptions, just to ignore this specific exception using the HTTP code/message to detect it.
I ignore all exception because my goal is to simulate the kubectl command behavior. This command just print an error message to alert users that all api-resources cannot be listed when an api is in error (whatever the error is). All other apis are present.
Here an example of kubectl command returns:
$ kubectl api-resources
NAME SHORTNAMES APIVERSION NAMESPACED KIND
bindings v1 true Binding
...
volumeattachments storage.k8s.io/v1 false VolumeAttachment
error: unable to retrieve the complete list of server APIs: bug.reproduce.java-client/v1alpha1: the server is currently unable to handle the request
I updated the warning message to be more like the one returned by the kubectl command.
I took the initiative of checking our respective implementations by running the program locally, and I confirm my previous analysis. Maybe if you're not convinced, you can check it locally too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the analysis.
The problem here is that the Discovery
class isn't just for Kubectl, it needs to be general purpose for any client, and if we catch an exception in the Discovery
class then we are obfuscating an error for a person who wants to use the Discovery
class outside of Kubectl
So emulating kubectl
behavior inside of the Kubectl
package is 100% fine, but other utility classes can't eat error messages.
I think that if you catch the exception here: https://github.com/kubernetes-client/java/blob/master/util/src/main/java/io/kubernetes/client/util/ModelMapper.java#L269 and return an empty Set I believe that it will have the same effect as adding the catch where you added it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if you catch the exception here: https://github.com/kubernetes-client/java/blob/master/util/src/main/java/io/kubernetes/client/util/ModelMapper.java#L269 and return an empty Set I believe that it will have the same effect as adding the catch where you added it.
An empty Set at this location will not call addModelMap and will lead to an exception thrown by the Kubectl client as already explained. Do you agree with that?
The problem here is that the
Discovery
class isn't just for Kubectl, it needs to be general purpose for any client, and if we catch an exception in theDiscovery
class then we are obfuscating an error for a person who wants to use theDiscovery
class outside ofKubectl
So emulating
kubectl
behavior inside of theKubectl
package is 100% fine, but other utility classes can't eat error messages.
Ok, I didn't understood that. So, in my opinion we have two solutions to fix this issue:
- Create a new checked exception, which contains a reference to all dicovered api resources, and will be thrown by discovery class. To ensure that the api contract is not broken by adding this new exception, it will extend ApiException. In library's code, this exception will be catched explicitly with api resources got from it. For user point of view, they will get an ApiException like before change.
- Update ModelMapper's methods to make them failover to prebuild metadata. All informations needed for this are not available in prebuild metadata
java/util/src/main/java/io/kubernetes/client/util/ModelMapper.java
Lines 329 to 331 in 7f95758
public static Boolean isNamespaced(Class<?> clazz) { return isNamespacedByClasses.get(clazz); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the additional thought here. I'm ok with adding an adding a specialized checked exception here to handle this case.
I think it is also valuable to implement the fallback in the ModelMapper that you describe (I believe that this is what the Golang client does in kubectl
) but I agree that it is extra work for this specific problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commited changes, would you please check it?
Code looks good, looks like you need to fix formatting. Please run |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, rossignolloic The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
fix #2719
Exceptions raised when calling an API to obtain all the resources it manages are now ignored. So when an API is unavailable, discovery doesn't stop, and all others APIs are processed.
This PR means that when ModelMapper calls Discovery#findAll and at least one api is unavailable, all metadata related to resources managed by unavailable APIs will not be updated in ModelMapper until the next refresh (30 minutes by default).
In addition, this PR allows the KubectlApiResources class (the extended client equivalent of
kubectl api-resources
) to list all resources managed by available API .